Skip to content

Conversation

@bojeil-google
Copy link
Contributor

Adds multi-factor support for updateUser
This will allow the ability to update the list of second factors on an existing user record.

Adds related unit and integration tests for this operation.

This will allow the ability to update the list of second factors on an existing user record.

Adds related unit and integration tests for this operation.
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Just a couple of nits and questions.

phoneNumber?: string | null;
photoURL?: string | null;
multiFactor?: {
enrolledFactors: Array<{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the SecondFactor type as used in UserImportBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a local SecondFactor type. Didn't want to risk getting in a possible circular dependency situation.

}
// Construct mfa related user data.
if (validator.isNonNullObject(request.multiFactor)) {
if (request.multiFactor.enrolledFactors === null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is slightly more clear:

request.mfa = {};
const enrollments = [];
if (validator.isArray(request.multiFactor.enrolledFactors)) {
   // iterate and push
}

if (enrollments.length > 0) {
  request.mfa.enrollments = enrollments;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it is a bit more complicated as passing request.mfa = {} will clear all existing second factors on the user.
We should only pass that if the developers passing explicitly an empty array for enrolledFactors or null for enrolledFactors.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a question.

tenantId?: string | null;

multiFactor?: {
enrolledFactors: Array<{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to reduce this duplication by introducing an interface? Like the SecondFactor type used in the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we can define an interface for this. I can do it in the next PR. This is going to be modified and more documentation added later anyway.

@bojeil-google bojeil-google merged commit 70b7da9 into auth-mfa Nov 19, 2019
@bojeil-google bojeil-google deleted the auth-mfa-temp branch November 20, 2019 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants